Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Two negating pins for dependencies - libnetcdf and numpy #5274

Merged
merged 4 commits into from
Apr 25, 2023
Merged

Two negating pins for dependencies - libnetcdf and numpy #5274

merged 4 commits into from
Apr 25, 2023

Conversation

trexfeathers
Copy link
Contributor

@trexfeathers trexfeathers commented Apr 24, 2023 •
edited
Loading

🚀 Pull Request

libnetcdf !=4.9.1

Closes #5245, closes #5187. Thanks very much @zklaus for sorting this out in conda-forge/libnetcdf-feedstock#175 🌟. I've tested locally and stdout is no longer polluted with numerous HDF5 errors.

numpy !=1.24.3

Updating the lock files introduced errors in iris.tests.test_analysis.TestLatitudeWeightGeneration:

  • test_cosine_latitude_weights_2d
  • test_cosine_latitude_weights_2d_latitude_first
  • test_cosine_latitude_weights_2d_latitude_last

Breadcrumb trail starts with numpy/numpy#23651. This relates to ordering in masked arrays, and having looked at the way the tests are constructed, I don't believe they are asserting for the wrong answer.

Consult Iris pull request check list

@trexfeathers
Copy link
Contributor Author

numpy !=1.24.3

I can't be 100% sure if the below was originally written accounting for broken NumPy behaviour that has now been fixed. Anyone care to weigh in?

iris/lib/iris/analysis/cartography.py

Lines 559 to 569 in 1afae0d

# Create weights for each grid point. This operation handles adding extra
# dimensions and also the order of the dimensions.
broadcast_dims = [x for x in lat_dims if x is not None]
wshape = []
for idim, dim in enumerate(lat_dims):
if dim is not None:
wshape.append(l_weights.shape[idim])
l_weights = l_weights.reshape(wshape)
broad_weights = iris.util.broadcast_to_shape(
l_weights, cube.shape, broadcast_dims
)

@codecov
Copy link

codecov bot commented Apr 24, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (1afae0d) 89.32% compared to head (c03fde8) 89.32%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5274   +/-   ##
=======================================
  Coverage   89.32%   89.32%           
=======================================
  Files          89       89           
  Lines       22390    22390           
  Branches     5374     5374           
=======================================
  Hits        20000    20000           
  Misses       1640     1640           
  Partials      750      750

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@trexfeathers
Copy link
Contributor Author

I can't be 100% sure if the below was originally written accounting for broken NumPy behaviour that has now been fixed. Anyone care to weigh in?

Although this would be rather odd, since NumPy's change is only for masked arrays, and if we had to put some sort of masked-vs-not type check in here that would seem like evidence that the problem is in NumPy and not in Iris.

bjlittle
bjlittle requested changes Apr 24, 2023
View reviewed changes
Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trexfeathers Just to confirm, from the PR description, are you saying that the lockfiles are generating issues with the new dependencies?

requirements/py310.yml Show resolved Hide resolved
@trexfeathers
Copy link
Contributor Author

@trexfeathers Just to confirm, from the PR description, are you saying that the lockfiles are generating issues with the new dependencies?

No, sorry for lack of clarity.

  1. Sorting out the libnetcdf pin required updating the lock files.
  2. Updating the lock files newly introduced NumPy v1.24.3, since it was released very recently.
  3. NumPy v1.24.3 caused the test failures described above.

@bjlittle
Copy link
Member

@trexfeathers We can watch numpy and see whether the 1.24.3 avoidance resurfaces in their next minor/patch release 👍

@bjlittle bjlittle merged commit cb287ce into SciTools:main Apr 25, 2023
@bjlittle bjlittle mentioned this pull request Apr 25, 2023
Merged
@trexfeathers
Copy link
Contributor Author

Thanks @bjlittle 🙂

tkknight added a commit to tkknight/iris that referenced this pull request Apr 25, 2023
@tkknight
Merge remote-tracking branch 'upstream/main' into apidoc_improvements
f0dce12 
* upstream/main:
  Two negating pins for dependencies - libnetcdf and numpy (SciTools#5274)
  [pre-commit.ci] pre-commit autoupdate (SciTools#5275)
  updated installing instructions to use pip (SciTools#5273)
  remove sphinx-panel config as now longer needed (SciTools#5272)
@github-actions github-actions bot mentioned this pull request Apr 26, 2023
Closed
tkknight added a commit to tkknight/iris that referenced this pull request Apr 26, 2023
@tkknight
Merge remote-tracking branch 'upstream/main' into dark_theme
d3574c2 
* upstream/main:
  Adopt sphinx-apidoc and retire complex sphinxext (SciTools#5264)
  Two negating pins for dependencies - libnetcdf and numpy (SciTools#5274)
  [pre-commit.ci] pre-commit autoupdate (SciTools#5275)
@w-k-jones w-k-jones mentioned this pull request Jun 6, 2023
Closed
@trexfeathers trexfeathers deleted the unpin_libnetcdf branch May 3, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjlittle bjlittle bjlittle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unpin libnetcdf HDF5-DIAG warnings from libnetcdf v4.9.1
2 participants
@trexfeathers @bjlittle